Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DMED-119 - Spike: direktes Einbinden der edu-sharing-Suchumgebung in den "Lern-Store" der SVS #2933

Open
wants to merge 118 commits into
base: main
Choose a base branch
from

Conversation

bergatco
Copy link
Contributor

@bergatco bergatco commented Nov 29, 2023

Short Description

The aim of this spike is to evaluate how the edu-sharing search environment can best be integrated as a "learning store" within the school cloud network software.

Links to Ticket and related Pull-Requests

https://ticketsystem.dbildungscloud.de/browse/DMED-119
hpi-schul-cloud/dof_app_deploy#713
hpi-schul-cloud/schulcloud-server#4692
hpi-schul-cloud/schulcloud-client#3387
hpi-schul-cloud/oeh-search-etl#50

Links to deployments (dev environments) :
https://dmed-119-integration-of-search-environment.dbc.dbildungscloud.dev/
https://dmed-119-integration-of-search-environment.nbc.dbildungscloud.dev/
https://dmed-119-integration-of-search-environment.brb.dbildungscloud.dev/

Link to deployments (in test environment) :
https://test.mediathek.dev.dbildungsplattform.de/

Changes

Data-security

Deployment

New Repos, NPM packages or vendor scripts

Screenshots of UI changes

Checklist before merging

  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • PO: Any deviation from requirements was agreed with Product-Owner / ticket author / support-team
  • DEV: Every new component is implemented having accessibility in mind (e.g. aria-label, role property)

Notice: Please keep this Pull-Request as a Draft (or add WIP label), until it is ready to be reviewed

Copy link

sonarcloud bot commented Nov 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

sonarcloud bot commented Jan 29, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are accessibility issues in these changes.

Copy link

sonarcloud bot commented May 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5 Security Hotspots
0.0% Coverage on New Code (required ≥ 80%)
4.8% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link

sonarcloud bot commented Jul 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5 Security Hotspots
0.0% Coverage on New Code (required ≥ 80%)
4.8% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@@ -254,7 +100,8 @@ watchDebounced(
flex-direction: column;
justify-content: space-between;
width: 100%;
min-height: calc(100vh - var(--sidebar-item-height));
min-height: calc(100vh - var(--sidebar-item-height) - 105.58px);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have to work with fixed values here?

Comment on lines +88 to +113
<style scoped>
.main-container {
min-height: 0;
flex-grow: 1;
display: flex;
flex-direction: column;
gap: 20px;
}
.search-form {
height: 64px;
flex-shrink: 0;
padding: 10px 16px;
background: #f3f5f7;
}
.search-input {
width: 500px;
border-radius: 2px;
border: 1px solid var(--Secondary-v-secondary-base, #54616e);
background: var(--shades---v-white-base, #fff);
}
edu-sharing-app {
flex-grow: 1;
}
:deep(.v-field) {
height: 44px;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

muss so viel custom css sein?

Comment on lines +89 to +113
.main-container {
min-height: 0;
flex-grow: 1;
display: flex;
flex-direction: column;
gap: 20px;
}
.search-form {
height: 64px;
flex-shrink: 0;
padding: 10px 16px;
background: #f3f5f7;
}
.search-input {
width: 500px;
border-radius: 2px;
border: 1px solid var(--Secondary-v-secondary-base, #54616e);
background: var(--shades---v-white-base, #fff);
}
edu-sharing-app {
flex-grow: 1;
}
:deep(.v-field) {
height: 44px;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum so viel eigene styles?

@@ -39,15 +50,25 @@ const createH5pEditorProxy = () => {
return h5pEditorProxy;
};

const createEduSharingRepoProxy = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum benötigt man ein eduSharingProxy und ein eduSharingRepoProxy? Was ist der Unterschied zwischen beiden?

@@ -73,6 +98,20 @@ const createDevServerConfig = () => {
},
});

// Copy assets before the dev server starts
const __base = path.resolve(__dirname, "../..");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Es wäre super wenn der Code dafür noch unter einer Funktion mit bezeichnenden Namen geclustert wird. Ehrlich gesagt müsste ich aber noch mal über das warum es notwendig ist nachdenken. Bzw. hier wäre drüber reden hilfreich.

Sind die ganzen Änderung potentiel etwas das hinter ein feature flag liegen sollten?

event.preventDefault();
const data = new FormData(event.target);
const searchString = data.get("searchString");
const eduSharingApp = document.getElementsByTagName("edu-sharing-app")[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document.getElementsByTagName("edu-sharing-app")[0] = erstes Element von irgendwas externen klinkt jetzt erstmal nicht nach Langzeit Maintaince save.

document.body.appendChild(styles);

// retrieve the valid session ticket
(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich kann dir nicht sagen wie es aussehen muss. Aber die api zur Komponente zu importieren und beim mount ein async einzubauen klinkt erstmal nicht richtig.

envConfigModule.getEnv.EDU_SHARING__API_URL + "/rest",
};

const runtime = document.createElement("script");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createElement
setAttribut
appendChild
etc.. in einer vue Komponente klinkt nicht nach state of the art.

} from "@/eduSharingApi/v3";
import { $axios } from "@/utils/api";

export const useEduSharingApi = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nur so als Frage sollte es ein composal geben das -api heißt?
Ich würde erwarten das ein composal maximal fachlichkeit ausdrückt und das es eine Datenquelle dahinter gibt, kann sein muss aber nicht, selbst die Art der Quelle ob Browser Cookie, json, api request dahinter liegt.
Evtl. ist es nur das umbennen des Files.

/* tslint:disable */
/* eslint-disable */
/**
* HPI Schul-Cloud Server API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das hatten wir doch schon im backend.
Ich glaube das sollte SVS Backend API sein. Wobei das ja eigentlich eduSharing ist.

/* eslint-disable */
/**
* HPI Schul-Cloud Server API
* This is v3 of HPI Schul-Cloud Server. Checkout /docs for v1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hier das gleiche Minus HPI, + SVS

/* tslint:disable */
/* eslint-disable */
/**
* HPI Schul-Cloud Server API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

/* tslint:disable */
/* eslint-disable */
/**
* HPI Schul-Cloud Server API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

/* eslint-disable */
/**
* HPI Schul-Cloud Server API
* This is v3 of HPI Schul-Cloud Server. Checkout /docs for v1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and again

</transition>
</div>
<content-edu-sharing-footer class="content__footer" />
<EduSharingWrapper />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big bang, oder feature flag?
Was ist mit sowas wie stores die evtl. noch rum liegen?

Copy link

sonarcloud bot commented Sep 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
0.0% Coverage on New Code (required ≥ 80%)
8.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-extend-activation-time WIP someone is working on that
Projects
None yet
Development

Successfully merging this pull request may close these issues.